[#630] Two-phase book page turn with visible page underneath#655
[#630] Two-phase book page turn with visible page underneath#655realproject7 merged 4 commits intomainfrom
Conversation
Replaced sequential flip-out/flip-in with simultaneous two-page stack: - Outgoing page rendered on top (z-index 2) with backface-visibility hidden - Incoming page visible underneath, revealed as outgoing flips away - Shadow follows the fold line during the 500ms turn animation - Outgoing page rotates 120deg (not 90) for realistic page curl effect - Incoming page has subtle opacity fade-in from 0.4 to 1 - GPU-accelerated: only transform, opacity, box-shadow animated - Works on swipe, button click, and keyboard arrows Fixes #630 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The new two-page stack is the right direction, but the navigation timing now resets the scroll container before the outgoing page animates. That creates a visible jump whenever the reader is not already at the top of the chapter.
Findings
- [high]
scrollToTop()now runs immediately aftersetCurrentIdx(idx), while the outgoing page is still being rendered from the same scrollable container. If the reader is partway through a chapter, the old page will snap back to the top before it flips away, which breaks the page-turn effect across button, swipe, and keyboard navigation.- File:
src/components/ReadingMode.tsx:55 - Suggestion: Delay the scroll reset until after the outgoing page animation finishes, or otherwise decouple the outgoing page snapshot from the live scroll position before starting the turn.
- File:
Decision
Requesting changes because this revision regresses the core reading interaction for normal mid-chapter navigation, even though the stacked-page approach itself is correct.
scrollToTop() was running immediately, causing the outgoing page to snap to top before flipping. Now deferred to after the 500ms animation so the outgoing page maintains its scroll position during the turn. Addresses T2a review feedback on PR #655. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
Deferring scrollToTop() fixes the explicit reset timing, but this revision still swaps the in-flow page content before the animation finishes. Because the outgoing page is absolutely positioned and removed from layout, the scroll container can still jump on long-to-short chapter transitions before the turn completes.
Findings
- [high]
setCurrentIdx(idx)still runs immediately while the outgoing page is rendered withposition: absolute, so the scrollable height becomes the incoming chapter's height during the animation. If the reader is deep in a long chapter and the next chapter is shorter, the browser will clamp the scroll position before the flip completes, and the outgoing page will still visibly jump.- File:
src/components/ReadingMode.tsx:54 - File:
src/app/globals.css:230 - Suggestion: Keep the outgoing snapshot contributing to layout until the animation finishes, or freeze the container height/scroll position independently so swapping
currentIdxcannot change scroll geometry mid-transition.
- File:
Decision
Requesting changes because the current fix handles the explicit scrollToTop() call but not the remaining scroll-geometry jump on mismatched chapter heights, so the page-turn interaction is still unreliable in a common reading flow.
Captures and freezes the stack container's height before swapping content, preventing scroll clamp when transitioning from a long chapter to a shorter one. Height is released after animation completes. Addresses T2a review: outgoing page could jump on long-to-short chapter transitions because the absolute-positioned outgoing page no longer contributed to layout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
Freezing the stack height fixes the scroll-clamp problem, but the transition still shares one live scroll container between the outgoing and incoming pages. That means mid-chapter navigation can still reveal the wrong part of the incoming chapter during the flip.
Findings
- [high] The incoming page is rendered underneath in the same scrollable container, but
scrollToTop()still does not run until the 500ms timeout completes. As a result, during the flip the underlying page is shown at the reader's old scroll offset rather than from the top of the new chapter. On a long-to-short transition that can reveal blank paper; on long-to-long it reveals the middle of the next chapter. Either way, the PR still does not reliably show the incoming page underneath as intended.- File:
src/components/ReadingMode.tsx:59 - File:
src/components/ReadingMode.tsx:62 - File:
src/components/ReadingMode.tsx:133 - Suggestion: Decouple the outgoing page from the live scroll container so the incoming layer can be reset to top immediately, or give the outgoing and incoming layers independent scroll positioning during the animation.
- File:
Decision
Requesting changes because the height freeze addresses one scroll bug, but the current structure still cannot show the correct incoming page content underneath the flip for normal mid-chapter navigation.
Captures scrollTop before navigation, scrolls to top immediately for the incoming page, then renders the outgoing page with a negative top offset to maintain its visual scroll position during the flip. This fully decouples the two pages: incoming starts at top, outgoing animates from wherever the reader was — no blank space or mid-chapter jumps on either page. Addresses T2a review: outgoing/incoming shared scroll container issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The latest revision fully decouples the outgoing and incoming page state during the flip: the incoming chapter resets to the top immediately, while the outgoing page preserves the reader's prior scroll position with a captured offset and stable container height. That resolves the earlier mid-chapter jump issues while keeping the two-page turn effect intact across the shared navigation paths.
Findings
- No blocking findings.
Decision
Approving because the current revision addresses the prior review feedback and now matches the intended two-phase page-turn behavior for Reading Mode.
2 participants
Summary
backface-visibility: hidden, and incoming page fade-in.Fixes #630
Tracks realproject7/agent-os#318
Test plan
🤖 Generated with Claude Code